-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AArch64 support for RSEQ. #50
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert on rseq, but I've highlighted a few areas where I believe the asm sequences could be tighter.
We'll also need to get the CLA fixed before merging. |
I have fixed the indentation problem pointed out by Owen. The other comments Owen provided are either blocked because of the need to not clobber argument registers in the critical section or me not having found a valid addressing mode that would improve performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* size_t len (w3) { | ||
* uint64_t x8 = __rseq_abi.cpu_id | ||
* uint64_t* x8 = CpuMemoryStart(x0, r8) | ||
* Header* hdr = x8 + w1 * 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let's note that we're using x4
for hdr
During some internal testing, I noticed a need to make sure TCMalloc is defining its own rseq-related macros (rather than requiring internal functions to follow TCMalloc's implementation's in lock-step). I should have a commit landed in the tree soon to rename those, but that'll require rebasing... |
I'll wait for that commit then. |
827107a
to
959e987
Compare
Had messed up the condition, now this should work, skip the test if the version < 9. For versions 9 or newer it will still check it is using the small memory model and error otherwise. |
PiperOrigin-RevId: 337894411 Change-Id: I372c99c367bd44ae6d096fc1f3219359f3395a94
PiperOrigin-RevId: 337894411 Change-Id: I372c99c367bd44ae6d096fc1f3219359f3395a94
Hi all,
This is a patch for initial RSEQ support for AArch64.
Tested on Ubuntu with 5.1.8+ kernel:
CC=clang bazel test -c dbg --copt "-mcpu=neoverse-n1" --linkopt "-fuse-ld=lld" --sandbox_debug --override_repository="tcmalloc=src/tcmalloc" -k //tcmalloc/..
CC=clang bazel test -c opt --copt "-mcpu=neoverse-n1" --linkopt "-fuse-ld=lld" --sandbox_debug --override_repository="tcmalloc=src/tcmalloc" -k //tcmalloc/..
On both occasions all tests ran and percpu_tcmalloc_test ran without skipping for slow path.
Is this OK?
I am currently looking at adding BTI to protect the branch targets and maybe even PAC for when we spill the LR onto the stack in START_RSEQ. Do you have any objections to this? I noticed the other ports don't use anything similar.
Another question I had is whether you might be open to re-using linux headers to specify these RSEQ sequences, like https://github.com/compudj/librseq/blob/master/include/rseq/rseq-arm64.h
I believe they lead to pretty much the same, but might help make it easier to rewrite the code in case things change in the future?
Kind regards,
Andre